Add `GetPlatformInfo` action #32

nikkita1267 · 2020-05-14T10:16:36Z

This pull request implements the GetPlatformInfo action, closes #6.

I left libc_ver and install_date fields blank because function Uname.FromCurrentSystem (link) doesn't fill them. I also add libc crate which provides uname system call.

All fields, except libc_ver and install_date, are currently filled for Linux.

googlebot · 2020-05-14T10:16:47Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

nikkita1267 · 2020-05-14T19:44:36Z

@googlebot I signed it!

googlebot · 2020-05-14T19:44:44Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

panhania · 2020-05-17T18:41:25Z

src/action/platform_info.rs

+extern crate sys_info;
+
+/// Using for `uname` syscall.
+extern crate libc;


Why do you need these declarations?

No need now, I removed them.

src/action/platform_info.rs

panhania · 2020-05-17T18:43:46Z

src/action/platform_info.rs

+            CannotGetOSType(ref error) => {
+                write!(fmt, "can't get OS type error: {}", error)
+            },
+


Excessive blank line.

panhania · 2020-05-17T18:46:19Z

Cargo.toml

@@ -15,3 +15,4 @@ rrg-proto = { path = "proto/" }
 simplelog = { version = "0.7.6" }
 structopt = { version = "0.3.12" }
 sys-info = { version = "0.5.1" }


Recently, I have made a change to use the sysinfo crate rather than sys-info. sys-info does not seem to work on other operating systems and is more low-level, whereas sysinfo gives you more abstractions and works on pretty much all supported platforms and saves us from having a lot of unsafe calls. Have you considered using it?

Right now, the code will not compile on Windows and macOS, I believe.

I try to find something useful for my action in crate sysinfo, but, seems to me, this package doesn't provide any information about OS, only about system at all.
Also as documentation of sys-info says that it supports Windows and Mac OS too.

panhania · 2020-05-17T18:47:51Z

src/action/platform_info.rs

+/// A Response type for `GetPlatformInfo` action
+pub struct Response {
+    /// Client platform information
+    platform_information: PlatformInfo,


Is there a reason to have separate types PlatformInfo and Response (that is specific to an action called GetPlatformInfo)?

No reason, now fixed:)

panhania · 2020-05-17T18:48:45Z

src/action/platform_info.rs

+}
+
+/// Funcion that converts raw C-strings into `String` type
+#[inline(always)]


Any reason for forcing inlinining over letting the compiler decide (or a slightly less "demanding" variant: just #[inline])?

I thought that it would be nice if the conversion code was inlined, but probably this solution should be left to the compiler. Thanks!

panhania · 2020-05-17T18:52:01Z

src/action/platform_info.rs

+    system: Option<String>,
+    release_name: Option<String>,
+    version_id: Option<String>,
+    machine: Option<String>,
+    kernel_release: Option<String>,
+    fqdn: Option<String>,
+    architecture: Option<String>,
+    node: Option<String>


It feels like a lot of these variants would benefit from having a dedicated enum rather than using raw strings for everything.

I don't quite understand, what do you mean by dedicated enum?

For example, for system you could have an enum with Linux, Windows etc. But this might not be ideal solution as it might break on some awkward platforms (that we don't really support, but who knows). On the other hand, passing through library-dependent values is not great either as we bind semantics of our protocol to some third-party internal details. In the end, I think it is fine to leave it as it is, I might look into that in the future.

panhania · 2020-05-30T21:41:38Z

src/action/mod.rs

 //! instance of the corresponding request type and send some (zero or more)
 //! instances of the corresponding response type.

+pub mod platform_info;


Just platform.

This doesn't work, because my file is named platform_info.rs and if I change platform_info to platform I get compilation error. Or, maybe, I don't understand you correctly.

I mean, you should rename the module (both the file and this line) to be just platform.

panhania · 2020-05-31T19:27:17Z

src/action/platform_info.rs

+#[cfg(target_os = "linux")]
+use libc::{uname, utsname};
+use libc::c_char;


Try to put platform-specific imports locally next to the places that actually need them.

Ok, thanks! Fixed.

panhania · 2020-05-31T19:31:47Z

src/action/platform_info.rs

+            system: self.system.clone(),
+            release: self.release_name.clone(),


Why do you need to clone? into_proto takes an owned response.

I need to clone, because pep425tag also try to move these fields.

Why? You are just stringifying them, borrowing them should be enough. You probably need to refactor it to a separate variable.

panhania · 2020-05-31T19:32:18Z

src/action/platform_info.rs

+            machine: self.machine,
+            kernel: self.kernel_release,
+            fqdn: self.fqdn,
+            architecture: self.architecture.clone(),


panhania · 2020-05-31T19:33:18Z

src/action/platform_info.rs

+            pep425tag: Some(
+                format!("{}_{}_{}", 
+                    self.system.unwrap_or(String::from("")), 
+                    self.release_name.unwrap_or(String::from("")), 
+                    self.architecture.unwrap_or(String::from(""))
+            )),


If any of these values is None you could end up with something like foo__bar or _foo_. Is this expected?

I decided to fill this field by analogy with the Python code, but forgot that None converts to 'None' in Python. Thanks!

panhania · 2020-05-31T19:36:59Z

src/action/platform_info.rs

+}
+
+#[cfg(test)]
+mod test {


These tests mostly re-implement the implementation, which makes them quite useless (or even dangerous). In most cases, its of course impossible to write proper tests, so I think it is just better to assert that the values are non-empty.

Ok, understand, thanks!

panhania · 2020-06-01T13:29:42Z

src/action/mod.rs

 //! instance of the corresponding request type and send some (zero or more)
 //! instances of the corresponding response type.

+pub mod platform_info;


I mean, you should rename the module (both the file and this line) to be just platform.

panhania · 2020-06-01T13:31:19Z

src/action/platform_info.rs

+            system: self.system.clone(),
+            release: self.release_name.clone(),


Why? You are just stringifying them, borrowing them should be enough. You probably need to refactor it to a separate variable.

panhania · 2020-06-01T13:33:31Z

src/action/platform_info.rs

+    fn test_system() {
+        let mut session = session::test::Fake::new();
+        assert!(handle(&mut session, ()).is_ok());
+
+        assert_eq!(session.reply_count(), 1);
+        let platform_info = &session.reply::<Response>(0);
+
+        assert!(platform_info.system.is_some());
+    }


For this test you could actually have platform-specific test cases for every supported platform (through #[cfg_attr(..., ignore)]) that actually test this field for some specific values (e.g. "Linux").

panhania · 2020-06-01T13:44:24Z

src/action/platform_info.rs

+    system: Option<String>,
+    release_name: Option<String>,
+    version_id: Option<String>,
+    machine: Option<String>,
+    kernel_release: Option<String>,
+    fqdn: Option<String>,
+    architecture: Option<String>,
+    node: Option<String>


For example, for system you could have an enum with Linux, Windows etc. But this might not be ideal solution as it might break on some awkward platforms (that we don't really support, but who knows). On the other hand, passing through library-dependent values is not great either as we bind semantics of our protocol to some third-party internal details. In the end, I think it is fine to leave it as it is, I might look into that in the future.

panhania · 2020-06-01T13:45:16Z

src/action/platform_info.rs

+    }
+}
+
+/// A Response type for `GetPlatformInfo` action


"Response" should not be uppercase here, there should be a dot at the end of the comment.

panhania · 2020-06-01T13:45:46Z

src/action/platform_info.rs

+    /// Client platform information
+    system: Option<String>,


This documentation item looks like something that applies to the whole struct, not only to the system field.

panhania · 2020-06-01T13:48:53Z

src/action/platform_info.rs

+        "Linux" => {
+            get_linux_response(session, os_type)?;
+        },


This looks like a good candidate for conditional compilation rather than some run-time behaviour that depends on what the third-party library decides to return.

nikkita1267 added 6 commits May 11, 2020 08:30

Implement GetPlatformInfo action

2f061b5

Integrate action to mod.rs

9ff7962

Merge with upstream

b4e0ecb

Finish merge

bc441d6

Add tests and some comments

c3d9cfa

Add error handling

670195d

panhania requested changes May 17, 2020

View reviewed changes

nikkita1267 added 7 commits May 30, 2020 10:18

Sync with upstream

e644173

Some fixes

a30c6ba

Remove inline attribute

b001985

Update sys_info version

d5ae5b2

Fix compilation under Windows

2aacbab

Fix tests

c3483a2

Fix comments and improve get_linux_response

df047b6

panhania reviewed May 31, 2020

View reviewed changes

nikkita1267 added 2 commits June 1, 2020 09:59

Change tests and pep425tag in into_proto function

283a416

Fix tests

43000ba

panhania reviewed Jun 1, 2020

View reviewed changes

Some minor fixes

49b0fd9

		system: self.system.clone(),
		release: self.release_name.clone(),

Add GetPlatformInfo action #32

Are you sure you want to change the base?

Add GetPlatformInfo action #32

Uh oh!

Conversation

nikkita1267 commented May 14, 2020

Uh oh!

googlebot commented May 14, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

nikkita1267 commented May 14, 2020

Uh oh!

googlebot commented May 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Add `GetPlatformInfo` action #32

Add `GetPlatformInfo` action #32